Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Deep linking for the notification page #1667

Merged
merged 54 commits into from
Dec 7, 2019
Merged

feat: Deep linking for the notification page #1667

merged 54 commits into from
Dec 7, 2019

Conversation

lei9444
Copy link
Contributor

@lei9444 lei9444 commented Nov 26, 2019 •
edited
Loading

Description

  • For dialog errors (eg. property field ), it will navigate back to the dialog and the error property field will be highlighted with red rectangle.
  • For lu errors, it will navigate back to the dialog and luis editor will be in red rectangle
  • For lg errors, it will navigate to the lg all up view in edit mode and focus on the error line

Task Item

refs #1582

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

dialog

lg
lu

@lei9444 lei9444 marked this pull request as ready for review November 27, 2019 07:51
compulim
compulim reviewed Nov 28, 2019
View reviewed changes
Composer/packages/client/src/ShellApi.ts Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/code-editor.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/language-generation/index.tsx Outdated Show resolved Hide resolved
Composer/packages/client/src/pages/notifications/NotificationList.tsx Outdated Show resolved Hide resolved
@lei9444
Copy link
Contributor Author

lei9444 commented Nov 28, 2019

Hi, @cwhitten @compulim Thanks for your review. I have resolved some of these comments. And I will add dialog's error link in this PR.

@lei9444 lei9444 requested a review from yeze322 as a code owner December 6, 2019 16:38
@lei9444
Copy link
Contributor Author

lei9444 commented Dec 6, 2019

image
Pagination is added to notification list now

@a-b-r-o-w-n a-b-r-o-w-n self-requested a review December 6, 2019 19:12
@cwhitten
Copy link
Member

cwhitten commented Dec 6, 2019

@lei9444 check out the following video: https://share.getcloudapp.com/Z4uwPKyg

I think we're still over-reporting empty fields as errors. The video shows me supplying incorrect expression syntax, but 4 errors being reported for it instead of 1. The 3 additional errors are the empty field errors that shouldn't be errors at all.

@lei9444
Copy link
Contributor Author

lei9444 commented Dec 7, 2019 •
edited
Loading

@lei9444 check out the following video: https://share.getcloudapp.com/Z4uwPKyg

I think we're still over-reporting empty fields as errors. The video shows me supplying incorrect expression syntax, but 4 errors being reported for it instead of 1. The 3 additional errors are the empty field errors that shouldn't be errors at all.

Hi, @cwhitten , It's a bug in checker function, I have fixed it now. I forget to clean the message and use the error message for all properties.
image

@cwhitten cwhitten dismissed stale reviews from a-b-r-o-w-n and themself December 7, 2019 15:48

stale

@cwhitten cwhitten merged commit 844df12 into microsoft:master Dec 7, 2019
This was referenced Dec 7, 2019
Merged
Closed
@hibrenda hibrenda mentioned this pull request Dec 9, 2019
Closed
6 tasks
@lei9444 lei9444 mentioned this pull request Dec 9, 2019
Merged
9 tasks
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request Dec 11, 2019
Merged
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request Dec 11, 2019
Merged
@lei9444 lei9444 deleted the deeplink branch January 17, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhixzhan zhixzhan zhixzhan left review comments

@boydc2014 boydc2014 boydc2014 left review comments

@compulim compulim compulim left review comments

@cwhitten cwhitten cwhitten approved these changes

@yeze322 yeze322 Awaiting requested review from yeze322

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n a-b-r-o-w-n is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lei9444 @cwhitten @compulim @a-b-r-o-w-n @boydc2014 @zhixzhan